Tailcall VM interrupt crash#21922
Conversation
| out($f," if (UNEXPECTED(((uintptr_t)opline & ZEND_VM_ENTER_BIT))) { \\\n"); | ||
| out($f," return opline; \\\n"); | ||
| out($f," } \\\n"); | ||
| out($f," ZEND_VM_TAIL_CALL(opline->handler(ZEND_OPCODE_HANDLER_ARGS_PASSTHRU)); \\\n"); |
There was a problem hiding this comment.
the alternative would be returning instead of tail-calling here. But that might be slower.
There was a problem hiding this comment.
Yes this would work. Possibly this may result in worse branch prediction as we revert to the main loop's central dispatch point.
Another alternative would be to make ZEND_VM_INTERRUPT() return an opline whose handler is zend_interrupt_helper, like we do for halt_op in the HYBRID and TAILCALL VMs: https://github.com/php/php-src/compare/arnaud-lb:vm-interrupt-tailcall-repro-3. This probably requires special handling in zend_jit_trace_execute(), I haven't investigated.
I've benchmarked the 3 approaches:
Results:
Symfony:
base: mean: 0.4459; stddev: 0.0004; diff: -0.00%
1 : mean: 0.4439; stddev: 0.0004; diff: -0.45%; p-value: 0.001000 (strong)
2 : mean: 0.4471; stddev: 0.0003; diff: +0.25%; p-value: 0.001000 (strong)
3 : mean: 0.4436; stddev: 0.0003; diff: -0.52%; p-value: 0.001000 (strong)
Symfony (valgrind):
base: diff: +0.00%
1 : diff: +0.12%
2 : diff: +0.01%
3 : diff: -0.00%
bench.php:
base: mean: 0.8044; stddev: 0.0004; diff: -0.00%
1 : mean: 0.8161; stddev: 0.0005; diff: +1.45%; p-value: 0.001000 (strong)
2 : mean: 0.8142; stddev: 0.0016; diff: +1.22%; p-value: 0.001000 (strong)
3 : mean: 0.8043; stddev: 0.0006; diff: -0.01%; p-value: 0.690382 (weak)
bench.php (valgrind):
base: diff: +0.00%
1 : diff: +1.03%
2 : diff: -0.50%
3 : diff: -0.00%
The Symfony results do not make sense to me, but these are stable.
Approach 3 seems better overall, speed-wise, assuming that we don't find issues with it.
There was a problem hiding this comment.
Uh, that's a nice approach! Love it.
I don't see any issues, that approach will never have &call_interrupt_op show up in EX(opline) or the VM IP register so that's perfect.
I have no idea about the JIT code, so, I'll leave that to you :-D
There was a problem hiding this comment.
I've pulled in your changes in the 3rd approach to let CI run while I review it in more dept (first glance looks good).
The call_interrupt_op sentinel approach only applies to TAILCALL VM,
where musttail constraints prevent checking ZEND_VM_ENTER_BIT in
ZEND_VM_DISPATCH_TO_HELPER. For CALL VM (non-global-reg), zend_interrupt
now forwards directly to zend_interrupt_helper_SPEC instead of returning
&call_interrupt_op.
Guard zend_interrupt (function and forward declaration) with:
#if !(defined(ZEND_VM_FP_GLOBAL_REG) && defined(ZEND_VM_IP_GLOBAL_REG))
&& ZEND_VM_KIND != ZEND_VM_KIND_TAILCALL
This fixes three failure modes:
- Global-reg builds (HYBRID/CALL with FP+IP regs): ZEND_OPCODE_HANDLER_RET
is void, making return &call_interrupt_op a constraint violation
- CALL non-global-reg builds (x32, Windows): call_interrupt_op was
undeclared for non-TAILCALL VM kinds
- TAILCALL builds: zend_interrupt (CALL variant) compiled but unused,
triggering -Werror=unused-function
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In TAILCALL delayed helpers (EX-signature helpers that can't musttail), ZEND_VM_INTERRUPT() was routing through zend_interrupt_helper_SPEC, which can return a tagged pointer (ZEND_VM_ENTER_BIT set) via ZEND_VM_ENTER(). That tagged pointer propagated back to ZEND_VM_DISPATCH_TO_HELPER, which blindly dispatched to it, causing a misaligned access crash. The fix: TAILCALL delayed helpers use SAVE_OPLINE() + return &call_interrupt_op, the sentinel pattern already used by TAILCALL VM. ZEND_VM_DISPATCH_TO_HELPER then dispatches to call_interrupt_op->handler (zend_interrupt_helper_SPEC_TAILCALL), which loads the real opline from EX(opline) and handles the interrupt cleanly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| if ($kind !== ZEND_VM_KIND_TAILCALL) { | ||
| /* Guard: exclude global-reg builds (void return) and TAILCALL builds (have own variant) */ | ||
| out($f, "#if !(defined(ZEND_VM_FP_GLOBAL_REG) && defined(ZEND_VM_IP_GLOBAL_REG)) && ZEND_VM_KIND != ZEND_VM_KIND_TAILCALL\n"); | ||
| } |
There was a problem hiding this comment.
We should be able to avoid this condition (or to simplify it), as gen_interrupt_func() is only called when $kind is ZEND_VM_KIND_CALL or ZEND_VM_KIND_TAILCALL.
| out($f, "#undef ZEND_VM_INTERRUPT\n"); | ||
| out($f, "#define ZEND_VM_CONTINUE(handler) return opline\n"); | ||
| out($f, "#define ZEND_VM_INTERRUPT() return zend_interrupt_helper".($spec?"_SPEC":"")."(ZEND_OPCODE_HANDLER_ARGS_PASSTHRU)\n"); | ||
| out($f, "#if ZEND_VM_KIND == ZEND_VM_KIND_TAILCALL\n"); |
There was a problem hiding this comment.
We should be able to avoid these conditions as this code is already generated inside an #if ZEND_VM_KIND == ZEND_VM_KIND_TAILCALL
There was a problem hiding this comment.
NB: There are already a few different #define ZEND_VM_INTERRUPT for the CALL, HYBRID (global regs), TAILCALL, and TAILCALL-but-cannot-tailcall cases. These can be adjusted if needed.
| } | ||
| out($f,"\n"); | ||
| out($f,"static ZEND_OPCODE_HANDLER_RET ZEND_OPCODE_HANDLER_FUNC_CCONV zend_interrupt_helper".($spec?"_SPEC":"")."(ZEND_OPCODE_HANDLER_ARGS);\n"); | ||
| out($f,"#if !(defined(ZEND_VM_FP_GLOBAL_REG) && defined(ZEND_VM_IP_GLOBAL_REG)) && ZEND_VM_KIND != ZEND_VM_KIND_TAILCALL\n"); |
There was a problem hiding this comment.
I think that this could be simplified to #if ZEND_VM_KIND == ZEND_VM_KIND_CALL
| out($f, "# define ZEND_VM_INTERRUPT() return zend_interrupt(ZEND_OPCODE_HANDLER_ARGS_PASSTHRU);\n"); | ||
| out($f, "#else\n"); | ||
| out($f, "#if ZEND_VM_KIND == ZEND_VM_KIND_TAILCALL\n"); | ||
| out($f, "# define ZEND_VM_INTERRUPT() SAVE_OPLINE(); return &call_interrupt_op;\n"); |
There was a problem hiding this comment.
This seems equivalent to return zend_interrupt_TAILCALL(ZEND_OPCODE_HANDLER_ARGS_PASSTHRU). The advantage of this form (return zend_interrupt_TAILCALL(...)) is that it reduces code size slightly compared to an inline SAVE_OPLINE() + return.
…acro The CALL variant of zend_interrupt was never called: delayed helpers only exist in TAILCALL builds ($delayed_helpers is only populated when $kind === ZEND_VM_KIND_TAILCALL), so the #elif/#else branches in the ZEND_VM_INTERRUPT macro were dead code, and zend_interrupt itself was compiled but unreferenced — triggering an unused-function warning on macOS ARM64 DEBUG NTS. Remove the CALL case from gen_interrupt_func, drop its forward declaration from the CALL section preamble, and replace the three-way C-level #if in the delayed helpers ZEND_VM_INTERRUPT macro with a single unconditional line (the TAILCALL sentinel path), since that block is only ever emitted for TAILCALL builds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In the new tailcall VM, if a call like
ZEND_VM_DISPATCH_TO_HELPER(...)uses a helper likezend_is_smaller_helper_SPEC_TAILCALL(), and during that helperEG(vm_interrupt)gets set, then it will return the current opline tagged withZEND_VM_ENTER_BIT. But theZEND_VM_DISPATCH_TO_HELPERmacro directly dereferenced the tagged pointer.This PR changes the macro to check for a tagged pointer, and returns instead of doing the tailcall as the outer executor will handle the
ZEND_VM_ENTER_BIT.To reproduce this, I piggy-backed on the vm_interrupt machinery in zend_test I just added in #21910. It uses a compare handler on
class VmInterruptComparableto deterministically set theEG(vm_interrupt).